Skip to content

Concurrent pantsd: Swap uncacheability for run-scoped results - #23616

Merged
tobni merged 6 commits into
pantsbuild:mainfrom
tobni:graph/cross-run-result-scoping
Nov 3, 2026
Merged

tobni merged 6 commits into
pantsbuild:mainfrom
tobni:graph/cross-run-result-scoping

Conversation

@tobni

@tobni tobni commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

NodeResult holds the producing RunId in place of an "uncacheable" bool. A Run that attaches to an in-flight attempt started by another Run re-enters get_node_result instead of taking that attempt's result.

Nothing can observe another Run's attempt under the pantsd concurrency lock, so this change is behaviour-preserving work.

See https://docs.google.com/document/d/1XEO55efGI8x91wGiVjmlFpUORJeQpZ2xF4zQoSE4TuY/edit?tab=t.0#heading=h.apz3211m77eg for design intent.

@tobni
tobni requested review from benjyw and tdyas August 9, 2026 11:17
@tobni tobni added category:internal CI, fixes for not-yet-released features, etc. release-notes:not-required [CI] PR doesn't require mention in release notes labels Aug 9, 2026
Comment thread src/rust/graph/src/entry.rs Resolved
Comment thread src/rust/graph/src/entry.rs Outdated

fn has_uncacheable_deps(&self) -> bool {
/// The Run which may consume this result, if it is confined to one.
fn uncacheable_run_id(&self) -> Option<RunId> {

@tdyas tdyas Aug 9, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uncacheable_run_id doesn't stand out to me as a very descriptive name. Maybe include some concept of the fact that it is the run ID for another consumer (going by the comment "may consume this result")?

Also, overall, what happens if there is more than one consumer of the result? (I'm reacting to the comment saying "if it is confined to one."

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed to run_scope. It's the Run that produced the result, not another consumer's. All of consumers within that Run share the result, but other runs will treat it as dirty, always.

Comment thread src/rust/graph/src/tests.rs Outdated
Comment on lines +1017 to +1018
let (entered_tx, entered_rx) = watch::channel(false);
let (release_tx, release_rx) = watch::channel(false);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why a Tokio watch instead of a one-shot channel? Watches would generally be used for passing around values like configuration data and are multi-producer, multi-consumer. Am I missing something?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was trying to avoid a mutex, which I now realize was not worth it. Switched!

tobni and others added 5 commits November 1, 2026 18:15
A Run which attaches to another Run's in-flight attempt cannot consume its
run-scoped result, so it re-enters to compute its own.
Names the result, generation and run_scope fields.
complete() sets the generation and run scope itself.
@tobni
tobni force-pushed the graph/cross-run-result-scoping branch from ea53f7a to 5a2f35f Compare November 1, 2026 17:18
@tobni
tobni force-pushed the graph/cross-run-result-scoping branch from 5a2f35f to 4fe1ae7 Compare November 1, 2026 17:23
@tobni
tobni requested a review from tdyas November 1, 2026 17:25
@tobni
tobni merged commit 8a3ecf2 into pantsbuild:main Nov 3, 2026
28 checks passed
@tobni
tobni deleted the graph/cross-run-result-scoping branch November 3, 2026 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category:internal CI, fixes for not-yet-released features, etc. release-notes:not-required [CI] PR doesn't require mention in release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants